-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hide message text input bar if the user cannot send messages in a room #253
Conversation
Screencast.from.2024-11-15.10-34-45.webmI will never give up, please review:) |
Untitled.video.-.Made.with.Clipchamp.mp4Here are some issues I found:
|
/ / My advice is to always check the behavior of the element.io for the correct api to use. Use two accounts, both in the same room. One mutes the other, you should already see the message text input bar hidden for the muted user. Hence there is no "can_user_send_message" checks in between. |
Done. |
@CodiumAI-Agent /review |
PR Reviewer Guide 🔍(Review updated until commit b700faa)Here are some key observations to aid the review process:
|
Nope, you need to also inspect the network using browser dev tool and understand matrix api.
|
Persistent review updated to latest commit b700faa |
@Demolemon11 Core requirements:
Implementation details:
Suggestions for improvement:
Code improvement suggestions:
|
this can be done in a future PR, too, if you want to keep this one simple/manageable. Multiple smaller PRs are preferred over one big PR.
Same comment as above.
I think this is the same thing as the first point. As I mentioned in one of my earlier comments, a subscription mechanism that asynchronously |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the naming fixes, things are much better now. Just a few more minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed a few things from my last review; make sure you're reading each comment in detail, and using GitHub's commit/batch suggestion feature whenever you agree with my suggested code change.
Also, can you post a video proving that your code here actually does catch a changed permission? That is, I'd like to see a screen recording in which you:
- in Robrix with user account A, open a room that is owned/admin'd by user account B.
- in a different client with user account B, change the permissions of user account A for that room to be either "can post" or "cannot post"
- show that Robrix with user A has changed the view of the input bar on the bottom
- in the different client, change user A's permissions again back to what they originally were
- show that Robrix has changed its view of that room back to the original.
result: bug: It seems there is a bug: |
Very nice, looks great!
Yeah that's the same issue as the issue you had with the verification badge not persisting across mobile <--> desktop UI changes. The state of the widget gets reset. You can fix it in a future PR or now, your choice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve this now. If you want to fix the issue mentioned above in this PR, just let me know, and I'll wait to merge it. If you want to fix it in a later PR, let me know and I can merge this one now.
Thanks, yeah those two should be easy enough to fix. The overflow of the permission message text can be fixed by setting its |
issue231